From eddc96ee2bc8935d57f8a9d7e43b4934a51bed8b Mon Sep 17 00:00:00 2001 From: Keir Fraser Date: Fri, 16 Nov 2007 20:06:15 +0000 Subject: [PATCH] Log dirty radix tree code cleanup. Also do not deference non-existent pointer in paging_new_log_dirty_*() functions if allocation fails. Signed-off-by: Keir Fraser --- xen/arch/x86/mm/paging.c | 125 +++++++++++++++++++------------ xen/arch/x86/mm/shadow/private.h | 8 +- 2 files changed, 80 insertions(+), 53 deletions(-) diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c index 3030c4b553..9de049aafd 100644 --- a/xen/arch/x86/mm/paging.c +++ b/xen/arch/x86/mm/paging.c @@ -101,36 +101,37 @@ static mfn_t paging_new_log_dirty_page(struct domain *d, void **mapping_p) mfn_t mfn; struct page_info *page = alloc_domheap_page(NULL); - if ( unlikely(page == NULL) ) { + if ( unlikely(page == NULL) ) + { d->arch.paging.log_dirty.failed_allocs++; return _mfn(INVALID_MFN); } + d->arch.paging.log_dirty.allocs++; mfn = page_to_mfn(page); *mapping_p = map_domain_page(mfn_x(mfn)); + return mfn; } - static mfn_t paging_new_log_dirty_leaf(struct domain *d, uint8_t **leaf_p) { mfn_t mfn = paging_new_log_dirty_page(d, (void **)leaf_p); - clear_page(*leaf_p); + if ( mfn_valid(mfn) ) + clear_page(*leaf_p); return mfn; } - static mfn_t paging_new_log_dirty_node(struct domain *d, mfn_t **node_p) { int i; mfn_t mfn = paging_new_log_dirty_page(d, (void **)node_p); - for (i = 0; i < LOGDIRTY_NODE_ENTRIES; i++) - (*node_p)[i] = _mfn(INVALID_MFN); + if ( mfn_valid(mfn) ) + for ( i = 0; i < LOGDIRTY_NODE_ENTRIES; i++ ) + (*node_p)[i] = _mfn(INVALID_MFN); return mfn; } - -/* allocate bitmap resources for log dirty */ int paging_alloc_log_dirty_bitmap(struct domain *d) { mfn_t *mapping; @@ -139,7 +140,8 @@ int paging_alloc_log_dirty_bitmap(struct domain *d) return 0; d->arch.paging.log_dirty.top = paging_new_log_dirty_node(d, &mapping); - if ( unlikely(!mfn_valid(d->arch.paging.log_dirty.top)) ) { + if ( unlikely(!mfn_valid(d->arch.paging.log_dirty.top)) ) + { /* Clear error indicator since we're reporting this one */ d->arch.paging.log_dirty.failed_allocs = 0; return -ENOMEM; @@ -149,45 +151,57 @@ int paging_alloc_log_dirty_bitmap(struct domain *d) return 0; } - static void paging_free_log_dirty_page(struct domain *d, mfn_t mfn) { d->arch.paging.log_dirty.allocs--; free_domheap_page(mfn_to_page(mfn)); } -/* free bitmap resources */ void paging_free_log_dirty_bitmap(struct domain *d) { + mfn_t *l4, *l3, *l2; int i4, i3, i2; - if (mfn_valid(d->arch.paging.log_dirty.top)) { - mfn_t *l4 = map_domain_page(mfn_x(d->arch.paging.log_dirty.top)); - printk("%s: used %d pages for domain %d dirty logging\n", - __FUNCTION__, d->arch.paging.log_dirty.allocs, d->domain_id); - for (i4 = 0; i4 < LOGDIRTY_NODE_ENTRIES; i4++) { - if (mfn_valid(l4[i4])) { - mfn_t *l3 = map_domain_page(mfn_x(l4[i4])); - for (i3 = 0; i3 < LOGDIRTY_NODE_ENTRIES; i3++) { - if (mfn_valid(l3[i3])) { - mfn_t *l2 = map_domain_page(mfn_x(l3[i3])); - for (i2 = 0; i2 < LOGDIRTY_NODE_ENTRIES; i2++) - if (mfn_valid(l2[i2])) - paging_free_log_dirty_page(d, l2[i2]); - unmap_domain_page(l2); - paging_free_log_dirty_page(d, l3[i3]); - } - } - unmap_domain_page(l3); - paging_free_log_dirty_page(d, l4[i4]); - } + if ( !mfn_valid(d->arch.paging.log_dirty.top) ) + return; + + dprintk(XENLOG_DEBUG, "%s: used %d pages for domain %d dirty logging\n", + __FUNCTION__, d->arch.paging.log_dirty.allocs, d->domain_id); + + l4 = map_domain_page(mfn_x(d->arch.paging.log_dirty.top)); + + for ( i4 = 0; i4 < LOGDIRTY_NODE_ENTRIES; i4++ ) + { + if ( !mfn_valid(l4[i4]) ) + continue; + + l3 = map_domain_page(mfn_x(l4[i4])); + + for ( i3 = 0; i3 < LOGDIRTY_NODE_ENTRIES; i3++ ) + { + if ( !mfn_valid(l3[i3]) ) + continue; + + l2 = map_domain_page(mfn_x(l3[i3])); + + for ( i2 = 0; i2 < LOGDIRTY_NODE_ENTRIES; i2++ ) + if ( mfn_valid(l2[i2]) ) + paging_free_log_dirty_page(d, l2[i2]); + + unmap_domain_page(l2); + paging_free_log_dirty_page(d, l3[i3]); } - unmap_domain_page(l4); - paging_free_log_dirty_page(d, d->arch.paging.log_dirty.top); - d->arch.paging.log_dirty.top = _mfn(INVALID_MFN); - ASSERT(d->arch.paging.log_dirty.allocs == 0); - d->arch.paging.log_dirty.failed_allocs = 0; + + unmap_domain_page(l3); + paging_free_log_dirty_page(d, l4[i4]); } + + unmap_domain_page(l4); + paging_free_log_dirty_page(d, d->arch.paging.log_dirty.top); + + d->arch.paging.log_dirty.top = _mfn(INVALID_MFN); + ASSERT(d->arch.paging.log_dirty.allocs == 0); + d->arch.paging.log_dirty.failed_allocs = 0; } int paging_log_dirty_enable(struct domain *d) @@ -369,39 +383,52 @@ int paging_log_dirty_op(struct domain *d, struct xen_domctl_shadow_op *sc) pages = 0; l4 = map_domain_page(mfn_x(d->arch.paging.log_dirty.top)); - for ( i4 = 0; pages < sc->pages && i4 < LOGDIRTY_NODE_ENTRIES; i4++ ) { + + for ( i4 = 0; + (pages < sc->pages) && (i4 < LOGDIRTY_NODE_ENTRIES); + i4++ ) + { l3 = mfn_valid(l4[i4]) ? map_domain_page(mfn_x(l4[i4])) : NULL; - for ( i3 = 0; pages < sc->pages && i3 < LOGDIRTY_NODE_ENTRIES; i3++ ) { - l2 = l3 && mfn_valid(l3[i3]) ? map_domain_page(mfn_x(l3[i3])) : NULL; - for ( i2 = 0; pages < sc->pages && i2 < LOGDIRTY_NODE_ENTRIES; i2++ ) { + for ( i3 = 0; + (pages < sc->pages) && (i3 < LOGDIRTY_NODE_ENTRIES); + i3++ ) + { + l2 = ((l3 && mfn_valid(l3[i3])) ? + map_domain_page(mfn_x(l3[i3])) : NULL); + for ( i2 = 0; + (pages < sc->pages) && (i2 < LOGDIRTY_NODE_ENTRIES); + i2++ ) + { static uint8_t zeroes[PAGE_SIZE]; unsigned int bytes = PAGE_SIZE; - l1 = l2 && mfn_valid(l2[i2]) ? map_domain_page(mfn_x(l2[i2])) : zeroes; + l1 = ((l2 && mfn_valid(l2[i2])) ? + map_domain_page(mfn_x(l2[i2])) : zeroes); if ( unlikely(((sc->pages - pages + 7) >> 3) < bytes) ) bytes = (unsigned int)((sc->pages - pages + 7) >> 3); - if ( likely(peek) ) { - if ( copy_to_guest_offset(sc->dirty_bitmap, pages >> 3, l1, bytes) != 0) { + if ( likely(peek) ) + { + if ( copy_to_guest_offset(sc->dirty_bitmap, pages >> 3, + l1, bytes) != 0 ) + { rv = -EFAULT; goto out; } } - if ( clean && l1 != zeroes ) clear_page(l1); - pages += bytes << 3; - if (l1 != zeroes) + if ( l1 != zeroes ) unmap_domain_page(l1); } - if (l2) + if ( l2 ) unmap_domain_page(l2); } - if (l3) + if ( l3 ) unmap_domain_page(l3); } unmap_domain_page(l4); - if (pages < sc->pages) + if ( pages < sc->pages ) sc->pages = pages; log_dirty_unlock(d); diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h index 915a5de20f..cdd46f9dc2 100644 --- a/xen/arch/x86/mm/shadow/private.h +++ b/xen/arch/x86/mm/shadow/private.h @@ -503,7 +503,7 @@ sh_mfn_is_dirty(struct domain *d, mfn_t gmfn) if ( unlikely(!VALID_M2P(pfn)) ) return 0; - if (d->arch.paging.log_dirty.failed_allocs > 0) + if ( d->arch.paging.log_dirty.failed_allocs > 0 ) /* If we have any failed allocations our dirty log is bogus. * Since we can't signal an error here, be conservative and * report "dirty" in this case. (The only current caller, @@ -515,19 +515,19 @@ sh_mfn_is_dirty(struct domain *d, mfn_t gmfn) l4 = map_domain_page(mfn_x(d->arch.paging.log_dirty.top)); mfn = l4[L4_LOGDIRTY_IDX(pfn)]; unmap_domain_page(l4); - if (!mfn_valid(mfn)) + if ( !mfn_valid(mfn) ) return 0; l3 = map_domain_page(mfn_x(mfn)); mfn = l3[L3_LOGDIRTY_IDX(pfn)]; unmap_domain_page(l3); - if (!mfn_valid(mfn)) + if ( !mfn_valid(mfn) ) return 0; l2 = map_domain_page(mfn_x(mfn)); mfn = l2[L2_LOGDIRTY_IDX(pfn)]; unmap_domain_page(l2); - if (!mfn_valid(mfn)) + if ( !mfn_valid(mfn) ) return 0; l1 = map_domain_page(mfn_x(mfn)); -- 2.30.2